-
Notifications
You must be signed in to change notification settings - Fork 74
Enable TensorIndexer with all python_direct tests #5781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Review updated until commit 5c9050d Description
|
| Relevant files | |||
|---|---|---|---|
| Enhancement |
| ||
| Tests |
|
PR Reviewer Guide
Here are some key observations to aid the review process:
| 🧪 PR contains tests |
| ⚡ Recommended focus areas for review |
Breaking Change Impact
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Enables the TensorIndexer (IdModel) by default for all Python direct tests by adding "id_model" as a default enable option in FusionDefinition.execute(). Simplifies the C++ option parsing logic in getIdModelEnabledOptions() by switching from an explicit opt-in model (checking for specific arguments like "consumer_index", "producer_index", etc.) to an opt-out model using "predicate_only" and "index_only" flags, removing redundant validation checks.
Confidence Score: 4/5
- Safe to merge - clean refactoring that enables IdModel by default for Python tests
- The changes are straightforward and follow the pattern established in the previous PR (#5724) that enabled TensorIndexer for C++ tests. The C++ logic simplification removes complex conditional checks and validation code, making it easier to maintain. The Python change simply adds "id_model" to the default enable options list, which is non-breaking since users can still override via _enable_options/_disable_options parameters. No logical errors or edge cases detected.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| csrc/id_model/utils.h | 4/5 | Simplifies IdModel option logic from explicit opt-in to opt-out with predicate_only/index_only flags |
| python/nvfuser_direct/init.py | 4/5 | Adds "id_model" as default enable option for all FusionDefinition.execute() calls |
Sequence Diagram
sequenceDiagram
participant Python as Python FusionDefinition.execute()
participant FEC as FusionExecutorCache
participant CPP as getIdModelEnabledOptions()
Python->>Python: "Add 'id_model' to default_enable_options"
Python->>FEC: "execute(merged_enable_options)"
FEC->>CPP: "Check IdModel enable option"
CPP->>CPP: "Check !predicate_only → Enable Index opts"
CPP->>CPP: "Check !index_only → Enable Predicate opts"
CPP->>CPP: "Check both unset → Enable Loop opt"
CPP-->>FEC: "Return enabled options set"
FEC-->>Python: "Execute with TensorIndexer enabled"
|
!test --diff |
|
!test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Enables TensorIndexer by default for all python_direct tests by making IdModel always build and removing the disable option. The PR removes the build_id_model_ flag from IdModelOptions and the DisableOption::IdModel enum, unconditionally building IdModel during lowering. Python's FusionDefinition.execute() now adds "id_model" as a default enable option.
Confidence Score: 2/5
- Breaking change to IdModel option semantics requires test updates
- The PR makes significant changes to how IdModel options are interpreted. The new
getIdModelEnabledOptions()function changes from an opt-in model (explicit arguments like "all", "index") to a negation-based model ("predicate_only", "index_only"). This breaks existing usage intests/python/direct/test_with_id_model_indexer.pywhich usesNVFUSER_ENABLE=id_model(all), but the "all" argument is no longer recognized. The old code would enable specific options based on explicit arguments, while the new code enables everything by default unless restricted. This semantic change warrants careful testing before merge. - csrc/id_model/utils.h - breaks backward compatibility with option arguments; tests/python/direct/test_with_id_model_indexer.py - needs update for new semantics
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| csrc/device_lower/id_model_options.h | 4/5 | Removes build_id_model_ flag and related methods, simplifying the options structure |
| csrc/device_lower/lower2device.cpp | 4/5 | Unconditionally builds IdModel by removing the conditional check for buildIdModel() |
| csrc/id_model/utils.h | 4/5 | Simplifies option logic by checking EnableOption::IdModel first and using simpler argument checks |
| python/nvfuser_direct/init.py | 4/5 | Adds "id_model" as default enable option for all fusion executions |
Sequence Diagram
sequenceDiagram
participant Python as Python FusionDefinition
participant FEC as FusionExecutorCache
participant Lower as GpuLower
participant IdModel as IdModel
participant Options as IdModelOptions
Python->>Python: "Add 'id_model' to enable options"
Python->>FEC: "execute(inputs, enable=['id_model'])"
FEC->>Lower: "analysis(fusion)"
Lower->>IdModel: "Unconditionally construct IdModel"
IdModel->>Options: "Initialize IdModelOptions"
Options->>Options: "Check EnableOption::IdModel"
alt IdModel not enabled
Options->>Options: "Return empty options"
else IdModel enabled
alt predicate_only arg
Options->>Options: "Enable predicates only"
else index_only arg
Options->>Options: "Enable indexing only"
else No restrictive args
Options->>Options: "Enable all options"
end
end
csrc/id_model/utils.h
Outdated
| inline std::unordered_set<IdModelEnableOption> getIdModelEnabledOptions() { | ||
| if (!isOptionEnabled(EnableOption::IdModel)) { | ||
| return {}; | ||
| } | ||
|
|
||
| std::unordered_set<IdModelEnableOption> opts; | ||
|
|
||
| if (hasEnableOptionArgument(EnableOption::IdModel, "consumer_index") || | ||
| hasEnableOptionArgument(EnableOption::IdModel, "index") || | ||
| hasEnableOptionArgument(EnableOption::IdModel, "all")) { | ||
| if (!hasEnableOptionArgument(EnableOption::IdModel, "predicate_only")) { | ||
| opts.insert(IdModelEnableOption::ConsumerIndex); | ||
| } | ||
|
|
||
| if (hasEnableOptionArgument(EnableOption::IdModel, "producer_index") || | ||
| hasEnableOptionArgument(EnableOption::IdModel, "index") || | ||
| hasEnableOptionArgument(EnableOption::IdModel, "all")) { | ||
| opts.insert(IdModelEnableOption::ProducerIndex); | ||
| } | ||
|
|
||
| if (hasEnableOptionArgument(EnableOption::IdModel, "inline_predicate") || | ||
| hasEnableOptionArgument(EnableOption::IdModel, "predicate") || | ||
| hasEnableOptionArgument(EnableOption::IdModel, "all")) { | ||
| if (!hasEnableOptionArgument(EnableOption::IdModel, "index_only")) { | ||
| opts.insert(IdModelEnableOption::InlinePredicate); | ||
| } | ||
|
|
||
| if (hasEnableOptionArgument(EnableOption::IdModel, "unswitch_predicate") || | ||
| hasEnableOptionArgument(EnableOption::IdModel, "predicate") || | ||
| hasEnableOptionArgument(EnableOption::IdModel, "all")) { | ||
| opts.insert(IdModelEnableOption::UnswitchPredicate); | ||
| } | ||
|
|
||
| if (hasEnableOptionArgument(EnableOption::IdModel, "loop") || | ||
| hasEnableOptionArgument(EnableOption::IdModel, "all")) { | ||
| if (!hasEnableOptionArgument(EnableOption::IdModel, "predicate_only") && | ||
| !hasEnableOptionArgument(EnableOption::IdModel, "index_only")) { | ||
| opts.insert(IdModelEnableOption::Loop); | ||
| } | ||
|
|
||
| // Loop requires ConsumerIndex, ProducerIndex, InlinePredicate and | ||
| // UnswitchPredicate | ||
| if (opts.find(IdModelEnableOption::Loop) != opts.end()) { | ||
| NVF_ERROR( | ||
| opts.find(IdModelEnableOption::ConsumerIndex) != opts.end(), | ||
| "ConsumerIndex required for Loop"); | ||
| NVF_ERROR( | ||
| opts.find(IdModelEnableOption::ProducerIndex) != opts.end(), | ||
| "ProducerIndex required for Loop"); | ||
| NVF_ERROR( | ||
| opts.find(IdModelEnableOption::InlinePredicate) != opts.end(), | ||
| "InlinePredicate required for Loop"); | ||
| NVF_ERROR( | ||
| opts.find(IdModelEnableOption::UnswitchPredicate) != opts.end(), | ||
| "UnswitchPredicate required for Loop"); | ||
| } | ||
|
|
||
| return opts; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new implementation changes the semantics of EnableOption::IdModel arguments. The old code supported explicit arguments like "all", "index", "predicate", "consumer_index", "producer_index", etc. The new code only recognizes "predicate_only" and "index_only" as restrictive flags.
This breaks existing usage like NVFUSER_ENABLE=id_model(all) found in tests/python/direct/test_with_id_model_indexer.py:183. Consider either:
- Updating the test to use
NVFUSER_ENABLE=id_model(no arguments, which enables everything by default) - Adding backward compatibility for the "all" argument
|
!test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR enables TensorIndexer with all Python direct tests by making IdModel always-on and simplifying its configuration.
Key Changes:
- Removes DisableOption::IdModel - IdModel is now always built during GPU lowering, cannot be disabled
- Simplifies IdModelOptions - Removes
build_id_model_flag and related methods, streamlines consistency checking - Updates enable option parsing - Replaces granular options (consumer_index, producer_index, etc.) with simpler
predicate_only/index_onlyflags - Migrates from ComputeAtMap to IdModel APIs - Changes predicate_compute.cpp to use
idModel().idGraph()instead ofcaMap()for getting concrete IDs and checking mappings - Enables id_model by default in Python API - Adds "id_model" to default enable options for FusionDefinition.execute()
- Updates test expectations - Adjusts expected variable names and generated CUDA code to match new indexing behavior
The changes are part of a larger effort to transition from ComputeAtMap to IdModel/TensorIndexer as the primary indexing mechanism.
Confidence Score: 5/5
- Safe to merge - well-tested refactoring that enables TensorIndexer consistently across Python tests
- This PR removes the ability to disable IdModel and simplifies its configuration. All changes are internally consistent: DisableOption::IdModel is removed from enum, options map, and all conditional checks. The migration from caMap to idModel APIs in predicate_compute.cpp follows established patterns seen elsewhere in the codebase. Test updates reflect expected behavior changes from the new indexing approach. The Python API change safely adds id_model to default options (duplicates are handled gracefully by the options map).
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| csrc/device_lower/id_model_options.h | 5/5 | Removes build_id_model option and related code, simplifying IdModelOptions to always build IdModel |
| csrc/device_lower/lower2device.cpp | 5/5 | Removes conditional IdModel building, now always builds IdModel during lowering |
| csrc/id_model/utils.h | 5/5 | Simplifies getIdModelEnabledOptions to use predicate_only/index_only flags instead of granular options |
| csrc/options.cpp | 5/5 | Removes id_model from DisableOptions map |
| csrc/options.h | 5/5 | Removes IdModel enum value from DisableOption |
| csrc/predicate_compute.cpp | 5/5 | Replaces caMap with idModel for getting concrete IDs and checking ID mappings |
| python/nvfuser_direct/init.py | 5/5 | Adds id_model to default enable options for Python API execution |
| tests/cpp/test_indexing.cpp | 5/5 | Updates expected index variable names in test (i98-i100 to i114-i116) |
| tests/python/direct/test_python_direct.py | 5/5 | Updates expected CUDA kernel code to match new simplified indexing patterns |
Sequence Diagram
sequenceDiagram
participant User
participant PythonAPI as Python FusionDefinition
participant Lower as GpuLower
participant IdModelOpts as IdModelOptions
participant IdModel
participant PredicateCompute
participant TensorIndexer
User->>PythonAPI: execute(inputs)
PythonAPI->>PythonAPI: Add "id_model" to enable_options
PythonAPI->>Lower: FusionExecutorCache.execute()
Lower->>IdModelOpts: Construct with options
Note over IdModelOpts: No longer checks DisableOption::IdModel<br/>Always initializes for TensorIndexer
Lower->>IdModel: Build IdModel (unconditional)
Note over IdModel: Previously conditional on buildIdModel()
IdModel-->>Lower: IdModel graphs built
Lower->>PredicateCompute: Generate predicates
PredicateCompute->>IdModel: getConcreteMappedId(id)
Note over PredicateCompute: Uses idModel().idGraph().toGroup()->front()<br/>Instead of caMap().getConcreteMappedID()
IdModel-->>PredicateCompute: concrete ID
PredicateCompute->>IdModel: Check ID mapping
Note over PredicateCompute: Uses idGraph().disjointValSets().strictAreMapped()<br/>Instead of caMap().areMapped()
IdModel-->>PredicateCompute: mapping result
PredicateCompute-->>Lower: Predicates with TensorIndexer
Lower-->>User: Compiled kernel
Greptile OverviewGreptile SummaryThis PR enables the TensorIndexer (IdModel) by default for all Python direct API tests by modifying Implementation approach:
Test updates:
Codebase integration:
Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant FusionDefinition
participant FusionExecutorCache
participant EnableOptionsGuard
User->>FusionDefinition: execute(inputs, _enable_options=[])
FusionDefinition->>FusionDefinition: Prepend "id_model" to _enable_options
Note over FusionDefinition: merged = ["id_model"] + _enable_options
FusionDefinition->>FusionExecutorCache: execute(inputs, _enable_options=merged)
FusionExecutorCache->>EnableOptionsGuard: set(IdModel) for each option
Note over EnableOptionsGuard: Options stored in map (duplicates overwritten)
EnableOptionsGuard->>FusionExecutorCache: Options configured
FusionExecutorCache->>FusionExecutorCache: Compile & execute with IdModel enabled
FusionExecutorCache-->>User: Return outputs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
| # Add "id_model" as a default enable option | ||
| default_enable_options = ["id_model"] | ||
| merged_enable_options = default_enable_options + _enable_options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical Logic Issue: Adding "id_model" to enable options without arguments has no effect.
The IdModel system requires explicit sub-options to be passed as arguments (e.g., "all", "index", "consumer_index", "producer_index", etc.). These are checked via hasEnableOptionArgument(EnableOption::IdModel, "arg") in csrc/id_model/utils.h:30-80.
When you add "id_model" here without arguments, the Python bindings call EnableOptionsGuard::getCurOptions().set(opt.value()) with an empty vector (see python/python_direct/runtime.cpp:284). This means no IdModel features are actually enabled.
Evidence from codebase:
- C++ tests use:
EnableOption::IdModel, {"all"}(tests/cpp/utils.cpp) - Python tests that need IdModel use:
set_env(NVFUSER_ENABLE="id_model(all)")(tests/python/direct/test_with_id_model_indexer.py:183)
Proposed solution:
The Python _enable_options API doesn't support passing arguments. You need to either:
- Modify the Python bindings to support syntax like
["id_model:all"]or similar - Use environment variable
NVFUSER_ENABLE="id_model(all)"in test setup - Add a new Python API parameter specifically for IdModel options
Without this fix, TensorIndexer will NOT be enabled with python_direct tests as the PR title claims.
|
!test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
| default_enable_options = ["id_model"] | ||
| merged_enable_options = default_enable_options + _enable_options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Potential duplicate options if user passes "id_model" in _enable_options. Consider checking if "id_model" is already in _enable_options before adding it, or use a set to avoid duplicates. While this doesn't cause functional issues (the second set() call just overwrites), it's cleaner to avoid duplicates.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| # Add "id_model" as a default enable option | ||
| default_enable_options = ["id_model"] | ||
| merged_enable_options = default_enable_options + _enable_options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] No opt-out mechanism provided. Users cannot disable id_model when using execute() even if they want to. Consider either: (1) checking if "id_model" is in _disable_options and skipping the default, or (2) providing a parameter to control default options. This limits flexibility for users who may need to test without id_model or work around potential id_model bugs.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
| # Add "id_model" as a default enable option | ||
| default_enable_options = ["id_model"] | ||
| merged_enable_options = default_enable_options + _enable_options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding "id_model" as a default enable option means users cannot opt-out of this behavior through the Python API. Consider whether this is intentional or if there should be a mechanism to allow users to disable id_model for testing/debugging purposes.
Potential implications:
- Users performing A/B testing between old and new indexing cannot easily do so
- Debugging issues specific to id_model becomes harder without a way to disable it
- This is a behavioral change that affects all python_direct users
If this is intentional (to force id_model adoption), consider documenting this breaking change clearly. If not, consider checking if "id_model" is already in _enable_options or providing an opt-out mechanism.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No files reviewed, no comments
No description provided.